fix(webapp): null-safe getTime() calls in replication service for createdAt/updatedAt#18
fix(webapp): null-safe getTime() calls in replication service for createdAt/updatedAt#18deepshekhardas wants to merge 1 commit into
Conversation
🧭 Helm Chart Prerelease PublishedVersion: Install: helm upgrade --install trigger \
oci://ghcr.io/deepshekhardas/charts/trigger \
--version "4.5.0-rc.1-pr18.f27aa53"
|
There was a problem hiding this comment.
6 issues found across 234 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".changeset/bundle-skills-single-pass.md">
<violation number="1" location=".changeset/bundle-skills-single-pass.md:5">
P1: Changeset description does not match the PR changes. The PR fixes null-safe `getTime()` calls in the ClickHouse replication service, but the changeset describes an unrelated fix about `chat.agent` skills in `trigger dev`. This will generate an incorrect changelog entry and mislead users about what was fixed.</violation>
</file>
<file name=".github/workflows/changesets-pr.yml">
<violation number="1" location=".github/workflows/changesets-pr.yml:91">
P2: The no-PR guard misses the `null` case from `gh --jq`, so the step can continue with an invalid PR number and fail.</violation>
<violation number="2" location=".github/workflows/changesets-pr.yml:97">
P1: This auto-pass check is reported as success without validating the PR contents, so required checks can be bypassed if the release branch contains unexpected code changes.</violation>
</file>
<file name="apps/webapp/app/routes/api.v1.orgs.ts">
<violation number="1" location="apps/webapp/app/routes/api.v1.orgs.ts:27">
P3: The `if (!orgs)` guard is unreachable after `findMany()` and should be removed or changed to a length check.</violation>
</file>
<file name="apps/webapp/app/models/vercelIntegration.server.ts">
<violation number="1" location="apps/webapp/app/models/vercelIntegration.server.ts:1121">
P1: This delete-before-create upsert path can drop a valid env var if create fails, causing outages for the affected target.</violation>
</file>
<file name=".changeset/resource-catalog-runtime-registration.md">
<violation number="1" location=".changeset/resource-catalog-runtime-registration.md:6">
P1: The changeset describes a fix for `COULD_NOT_FIND_EXECUTOR` in runtime workers, but the PR title states this is about null-safe `getTime()` calls in the ClickHouse replication service. These are unrelated issues. When published, this changeset will generate a misleading changelog entry that doesn't match the PR's actual changes.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 100 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Re-trigger cubic
| "trigger.dev": patch | ||
| --- | ||
|
|
||
| Fix `chat.agent` skills silently missing in `trigger dev` for projects whose task files read `process.env` at module top level (e.g. a third-party SDK client initialized at import). Skill folders now bundle into `.trigger/skills/` reliably regardless of which env vars are set when the CLI launches. |
There was a problem hiding this comment.
P1: Changeset description does not match the PR changes. The PR fixes null-safe getTime() calls in the ClickHouse replication service, but the changeset describes an unrelated fix about chat.agent skills in trigger dev. This will generate an incorrect changelog entry and mislead users about what was fixed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .changeset/bundle-skills-single-pass.md, line 5:
<comment>Changeset description does not match the PR changes. The PR fixes null-safe `getTime()` calls in the ClickHouse replication service, but the changeset describes an unrelated fix about `chat.agent` skills in `trigger dev`. This will generate an incorrect changelog entry and mislead users about what was fixed.</comment>
<file context>
@@ -0,0 +1,5 @@
+"trigger.dev": patch
+---
+
+Fix `chat.agent` skills silently missing in `trigger dev` for projects whose task files read `process.env` at module top level (e.g. a third-party SDK client initialized at import). Skill folders now bundle into `.trigger/skills/` reliably regardless of which env vars are set when the CLI launches.
</file context>
| Fix `chat.agent` skills silently missing in `trigger dev` for projects whose task files read `process.env` at module top level (e.g. a third-party SDK client initialized at import). Skill folders now bundle into `.trigger/skills/` reliably regardless of which env vars are set when the CLI launches. | |
| Fix null-safe `getTime()` calls in the ClickHouse replication service for `createdAt` and `updatedAt` in `toTaskRunInsertArray` and `toSessionInsertArray`, preventing `TypeError: Cannot read properties of undefined (reading 'getTime')` when task runs or sessions are synced before their timestamps are fully populated. |
| -f name="All PR Checks" \ | ||
| -f head_sha="$HEAD_SHA" \ | ||
| -f status=completed \ | ||
| -f conclusion=success \ |
There was a problem hiding this comment.
P1: This auto-pass check is reported as success without validating the PR contents, so required checks can be bypassed if the release branch contains unexpected code changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/changesets-pr.yml, line 97:
<comment>This auto-pass check is reported as success without validating the PR contents, so required checks can be bypassed if the release branch contains unexpected code changes.</comment>
<file context>
@@ -72,3 +73,27 @@ jobs:
+ -f name="All PR Checks" \
+ -f head_sha="$HEAD_SHA" \
+ -f status=completed \
+ -f conclusion=success \
+ -f 'output[title]=Auto-pass for changeset release PR' \
+ -f 'output[summary]=Required check auto-satisfied for changeset-release/main PRs. Full CI ran on the underlying commits before they landed on main.'
</file context>
| }); | ||
| } else { | ||
| await client.projects.createProjectEnv({ | ||
| await client.projects.batchRemoveProjectEnv({ |
There was a problem hiding this comment.
P1: This delete-before-create upsert path can drop a valid env var if create fails, causing outages for the affected target.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/webapp/app/models/vercelIntegration.server.ts, line 1121:
<comment>This delete-before-create upsert path can drop a valid env var if create fails, causing outages for the affected target.</comment>
<file context>
@@ -1115,28 +1115,26 @@ export class VercelIntegrationRepository {
- });
- } else {
- await client.projects.createProjectEnv({
+ await client.projects.batchRemoveProjectEnv({
idOrName: vercelProjectId,
...(teamId && { teamId }),
</file context>
| "trigger.dev": patch | ||
| --- | ||
|
|
||
| Fix `COULD_NOT_FIND_EXECUTOR` when a task's definition is loaded via `await import(...)` from inside another task's `run()`. The runtime workers now register such tasks with a sentinel file context, and the catalog logs a one-time warning per task id. |
There was a problem hiding this comment.
P1: The changeset describes a fix for COULD_NOT_FIND_EXECUTOR in runtime workers, but the PR title states this is about null-safe getTime() calls in the ClickHouse replication service. These are unrelated issues. When published, this changeset will generate a misleading changelog entry that doesn't match the PR's actual changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .changeset/resource-catalog-runtime-registration.md, line 6:
<comment>The changeset describes a fix for `COULD_NOT_FIND_EXECUTOR` in runtime workers, but the PR title states this is about null-safe `getTime()` calls in the ClickHouse replication service. These are unrelated issues. When published, this changeset will generate a misleading changelog entry that doesn't match the PR's actual changes.</comment>
<file context>
@@ -0,0 +1,6 @@
+"trigger.dev": patch
+---
+
+Fix `COULD_NOT_FIND_EXECUTOR` when a task's definition is loaded via `await import(...)` from inside another task's `run()`. The runtime workers now register such tasks with a sentinel file context, and the catalog logs a one-time warning per task id.
</file context>
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| PR_NUMBER=$(gh pr list --head changeset-release/main --json number --jq '.[0].number') | ||
| if [ -z "$PR_NUMBER" ]; then exit 0; fi |
There was a problem hiding this comment.
P2: The no-PR guard misses the null case from gh --jq, so the step can continue with an invalid PR number and fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/changesets-pr.yml, line 91:
<comment>The no-PR guard misses the `null` case from `gh --jq`, so the step can continue with an invalid PR number and fail.</comment>
<file context>
@@ -72,3 +73,27 @@ jobs:
+ GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+ run: |
+ PR_NUMBER=$(gh pr list --head changeset-release/main --json number --jq '.[0].number')
+ if [ -z "$PR_NUMBER" ]; then exit 0; fi
+ HEAD_SHA=$(gh pr view "$PR_NUMBER" --json headRefOid --jq '.headRefOid')
+ gh api -X POST repos/${{ github.repository }}/check-runs \
</file context>
| if [ -z "$PR_NUMBER" ]; then exit 0; fi | |
| if [ -z "$PR_NUMBER" ] || [ "$PR_NUMBER" = "null" ]; then exit 0; fi |
| if (!orgs) { | ||
| return json({ error: "Orgs not found" }, { status: 404 }); | ||
| } |
There was a problem hiding this comment.
P3: The if (!orgs) guard is unreachable after findMany() and should be removed or changed to a length check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/webapp/app/routes/api.v1.orgs.ts, line 27:
<comment>The `if (!orgs)` guard is unreachable after `findMany()` and should be removed or changed to a length check.</comment>
<file context>
@@ -2,36 +2,43 @@ import type { LoaderFunctionArgs } from "@remix-run/server-runtime";
- if (!orgs) {
- return json({ error: "Orgs not found" }, { status: 404 });
- }
+ if (!orgs) {
+ return json({ error: "Orgs not found" }, { status: 404 });
+ }
</file context>
| if (!orgs) { | |
| return json({ error: "Orgs not found" }, { status: 404 }); | |
| } | |
| if (orgs.length === 0) { | |
| return json({ error: "Orgs not found" }, { status: 404 }); | |
| } |
…#3519) Add optional chaining to getTime() calls on createdAt, updatedAt fields in runs and sessions replication services. When CDC sends rows before timestamps are fully populated, calling .getTime() on undefined crashes the replication service with a TypeError. - runsReplicationService.server.ts: null-safe updatedAt/createdAt in #prepareTaskRunInsert and #preparePayloadInsert - sessionsReplicationService.server.ts: null-safe createdAt/updatedAt in toSessionInsertArray
f27aa53 to
6858123
Compare
Fixes triggerdotdev#3519
Changes
Root cause
When task runs or sessions are synced before their timestamps are fully populated in the CDC message, .getTime()\ was called on \undefined, crashing the replication stream.
Summary by cubic
Make timestamp handling null-safe in the ClickHouse replication services to stop crashes when CDC messages lack createdAt/updatedAt. Stabilizes the replication stream and addresses triggerdotdev#3519.
updatedAt?.getTime() ?? Date.now()andcreatedAt?.getTime() ?? Date.now()inapps/webapp/app/services/runsReplicationService.server.ts.apps/webapp/app/services/sessionsReplicationService.server.ts.TypeError: Cannot read properties of undefined (reading 'getTime')during replication.Written for commit f27aa53. Summary will update on new commits.